[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
We suggest the following additional approver: wallrj

Assign the PR to them by writing /assign @wallrj in a comment when ready.

The full list of commands accepted by this bot can be found here.

/test e2e

@wallrj: The following tests failed, say /retest to rerun them all:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Given the size of this package, what's the need to use the version in k/k given it involves vendoring the entire codebase?

I went looking at ways to create a non-cryptographic hash of the Pilot resource, in response to #153 (comment)

• Initially wrote code to hash the JSON serialised pilot.
• Then looked at ES code and found ComputeNodePoolHash and DeepHashObject.
• ....which looked like Navigator code, but I couldn't understand why it was using spew and why there was an unused collisionCount parameter.
• It wasn't until I realised that this had been copied from Kubernetes that I went and looked at the git history there
• and found they'd avoided using JSON marshalling because it misses out the private attributes of the object being hashed.
• and that the collision count is something to do with Deployment controller bringing up multiple ReplicaSets during a rollout, with pods that have potentially identical hashes.

If DeepCopyObject had instead been imported from Kubernetes, I probably would have used it without bothering investigating....which would have saved some time, but I wouldn't have learned much from it.

It's been interesting to look at the history of DeepHashObject. They've fixed a bunch of bugs since it was written, which, if we had copied the code at an earlier stage, we might affected by:

• DeepHashObject should probably set DisableMethods and DisablePointerMethods for spew kubernetes/kubernetes#6265
• Make DeepHashObject function deterministic. kubernetes/kubernetes#4595
• Update spew to support better map key sorting. kubernetes/kubernetes#6681

So that, to me, is the reason that "A little copying" should be avoided, if possible.

And it turns out that the latest dep is super fast. I can run dep ensure && dep prune in less than 30 seconds on my laptop, even when depending on the k8s.io/kubernetes/util/hash package.
And the pruning works really well, ensuring that only the hash package files are added to ./vendor

And according to golang/dep#944, pruning will soon be performed by dep ensure so we won't even have to think about it.

I'm not particularly desperate to get this PR merged. But I think it demonstrates that we can re-use code from the kubernetes repo, without having to vendor the entire project.

I 100% agree that seeing history is useful - we should have a link to the 'source' of the package within that directory, which would have made it much easier for you to find.

I don't however see how if the ES controller used the k/k package already, it would have been easier to discover. On the contrary, it would have put the package you are trying to find (but are not aware of it's existence) further from you (right now it's just in our pkg/ directory).

Regarding bugs - yep agreed. But a link in the README I think would go a long way. dep won't notify us of changes to the upstream package even if we did depend upon it, so it would still have taken some test failures or user reports for us to pick up the bug, which would eventually lead us to our vendored package, at which point we could go look at the recent history and spot the bug there (this is largely the same process by which we'd debug and find a fix if we had depended upon the main upstream repo).

As an aside, some of my other concerns with dep prune are around caching. I will need to dig out the exact issue, but it's already been said that dep prune will invalidate dep verify style commands (which ensure vendor is in sync with Gopkg), which (to me) is a shame.

My tl;dr on this is that I think an additional file in this package that references it's source would suffice, unless there's something else I'm missing here 😄

@wallrj PR needs rebase

dep ensure now prunes by default

Let's revisit this when the 0.4.0 release is ready.